Support flashing in Secure Download Mode #990
Conversation
enody-carter
force-pushed
the
secure_download_mode
branch
4 times, most recently
from
e67bec9 to
5398109
Compare
enody-carter
force-pushed
the
secure_download_mode
branch
from
5398109 to
5d4f443
Compare
* With secure download enabled, the stub is not utilized so compressed flashing is unavailble. In this situation, use the uncompressed flashing commands. * If secure download is enabled, prevent flashing data to addresses below 0x8000 to prevent overwriting the bootloader and bricking the device
enody-carter
force-pushed
the
secure_download_mode
branch
from
5d4f443 to
067f040
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds support for flashing ESP32 devices in Secure Download Mode, which uses ROM UART download with restricted command availability. The implementation avoids using compression (which requires stub loader commands not available in Secure Download Mode) and instead uses standard FlashBegin/FlashData/FlashEnd commands. Additionally, the PR implements bootloader protection to prevent writes below address 0x8000 and disables watchdog disable, verify, and skip operations which rely on restricted commands.
Key Changes:
- Implements non-compressed flash operations using FlashBegin/FlashData/FlashEnd for Secure Download Mode
- Adds bootloader protection to reject writes below 0x8000 when Secure Download Mode is enabled
- Skips watchdog disable, verify, and skip operations in Secure Download Mode
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| espflash/src/target/flash_target/esp32.rs | Implements dual-path flashing logic (compressed with stub vs uncompressed without stub), updates watchdog disable to skip in Secure Download Mode, renames need_deflate_end to need_flash_end |
| espflash/src/flasher/mod.rs | Adds bootloader protection checks and warning messages for Secure Download Mode in both image and binary flashing methods |
| espflash/src/error.rs | Adds new error variant for bootloader protection violation |
| CHANGELOG.md | Documents the new Secure Download Mode support feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for segment in image_format.clone().flash_segments() { | ||
| if segment.addr < BOOTLOADER_PROTECTION_ADDR { | ||
| return Err(Error::SecureDownloadBootloaderProtection); | ||
| } | ||
| } |
There was a problem hiding this comment.
The image_format is cloned here to iterate segments for address validation, then the original is consumed later at line 964. This could be optimized by collecting segments once into a Vec, validating addresses, then flashing from the Vec. However, this would require changing how flash_segments works since it consumes self.
Consider refactoring to collect segments once and reuse them, or provide a non-consuming iterator method for flash_segments to avoid the clone.
There was a problem hiding this comment.
I think this is valid, but also begs the question whether we should split the check out into its own fn, as its duplicated in more than one place.
There was a problem hiding this comment.
I agree, I ended up not breaking it out initially as load_image_to_flash() and write_bins_to_flash() are close to duplicates right now. I'm happy break this out into a new fn and open a new PR to track a unification of these and introduce a --force flag if desired).
There was a problem hiding this comment.
Maybe we can do the deduplication in this PR and leave the --force flag for an upcoming PR?
| let mut data = segment.data.to_vec(); | ||
| let padding = (flash_write_size - (data.len() % flash_write_size)) % flash_write_size; | ||
| data.extend(repeat_n(0xff, padding)); | ||
| data |
There was a problem hiding this comment.
When compression is disabled, the data is manually padded here with 0xff bytes to align to flash_write_size. However, at line 229, the FlashData command is also configured with pad_to: flash_write_size, which causes the data_command function to apply padding again. This results in double padding.
The manual padding should be removed since the FlashData command handles padding automatically via the pad_to parameter.
| })?; | ||
| Ok(()) | ||
| }, | ||
| )?; |
There was a problem hiding this comment.
When FlashBegin is called (non-compression path), need_flash_end should be set to true to ensure FlashEnd is called in the finish() method. Without this, the flash operation won't be properly completed in Secure Download Mode.
Add self.need_flash_end = true; after the FlashBegin command, similar to what's done for the compression path at line 183.
| )?; | |
| )?; | |
| self.need_flash_end = true; |
There was a problem hiding this comment.
This is absolutely valid, messed up my rebase and caused this.
MabezDev
left a comment
MabezDev
left a comment
There was a problem hiding this comment.
Sorry, I meant to come back and see copilots output but I got distracted. I've resolved and added some comments on relevant review items.
| for segment in image_format.clone().flash_segments() { | ||
| if segment.addr < BOOTLOADER_PROTECTION_ADDR { | ||
| return Err(Error::SecureDownloadBootloaderProtection); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this is valid, but also begs the question whether we should split the check out into its own fn, as its duplicated in more than one place.
4 participants
Fixes 726
When secure download mode is enabled, the flashing happens via the ROM UART download which does not support all commands. Utilize standard FlashBegin, FlashData, and FlashEnd when the stub is not utilized for flashing. Additionally, do not attempt to disable the watchdog or verify the flash when in secure download mode.
This implementation was based esptool's implementation. I included the write protection for base address < 0x8000 to protect the boot loader. In esptool this is allowed via a --force flag, but no such flag is present in espflash, so I defer to you on what you would like to do regarding this protection.